GDBRemote: chdir ds2 itself when the platform working directory is set - #249
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1907ee084d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… directory onFileOpen and its siblings in FileOperationsMixin passed vFile paths straight through to Host::File, never consulting the working directory set via QSetWorkingDir, unlike qPlatform_shell and process launch. This is why lldbutil.wait_for_file_on_target's ls-then-Get() sequence diverged: the shell-based ls resolves relative paths against the platform working directory (qPlatform_shell carries an explicit cwd field), but the vFile:open packet that immediately follows carries no such field, so ds2 has no way to know it unless it tracks the field from QSetWorkingDir itself. Resolve relative vFile paths against each session's own _workingDirectory rather than ds2's actual process cwd: PlatformMain in Sources/main.cpp serves every connected platform client on its own thread with its own PlatformSessionImpl, so the process cwd is shared, mutable state across all of them, and mutating it per-session would let concurrent clients stomp on each other's working directory. onSetWorkingDirectory canonicalizes the incoming path to absolute the moment it's set (a relative path only means something relative to ds2's own starting directory, since ds2's cwd never changes), rather than storing it verbatim. This also keeps ProcessSpawner's own chdir() in the forked child working correctly for a relative QSetWorkingDir: storing the raw relative string and letting both the vFile join and the spawner's chdir() apply it independently would resolve it against two different bases and risk applying it twice. Verified against a rebuilt android-x86_64 ds2 on the local emulator: TestHelloWorld.test_with_attach_to_process_with_id_api passes (was failing with "unable to open source file"); a raw protocol probe confirms a relative QSetWorkingDir resolves once (no doubling) and that two concurrent sessions keep independent working directories; and the sanity/api/android categories show no regressions.
compnerd
force-pushed
the
compnerd/vfile-working-directory
branch
from
July 28, 2026 22:13
d9415ab to
1fcaa83
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Host::File's POSIX and Windows backends both call straight through to open()/CreateFileW, which already resolve a relative path against the calling process's own current directory. ds2 never kept its own cwd in sync with QSetWorkingDir, only handing the working directory to spawned debuggees via ProcessSpawner's own chdir() in the forked child.
This is why lldbutil.wait_for_file_on_target's ls-then-Get() sequence diverged for tests like TestHelloWorld's attach-by-id/name cases: the shell-based ls resolves relative paths against the platform working directory (qPlatform_shell carries an explicit cwd field), but the vFile:open packet that immediately follows carries no such field, so it fell back to resolving against wherever ds2's own process happened to be running from and failed with "unable to open source file".
Every QSetWorkingDir in captured protocol logs is preceded by a qPlatform_mkdir for that exact path, so the directory exists by the time this chdir() runs.
Verified against a rebuilt android-x86_64 ds2 on the local emulator: TestHelloWorld.test_with_attach_to_process_with_id_api now passes (was failing with "unable to open source file"), and the sanity/api/android categories show no regressions from ds2 changing its own cwd.